Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add repo URL support and testing for cocoapods #153

Conversation

johnmhoran
Copy link
Member

Reference: #143

@TG1999 This replaces the recently-closed PR 151. I look forward to your comments and questions.

Reference: package-url#143

Signed-off-by: John M. Horan <johnmhoran@gmail.com>
purl_data = PackageURL.from_string(purl)
name = purl_data.name

if name:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if name:
if not name:
return
return f"https://cocoapods.org/pods/{name}"



Reference: package-url#143

Signed-off-by: John M. Horan <johnmhoran@gmail.com>
@johnmhoran
Copy link
Member Author

@TG1999 Thank you for your comments. I've just pushed my responsive updates.

  • I added "pkg:cocoapods/": None, to test_purl2url_get_repo_url() and added "pkg:cocoapods/": [], to test_purl2url_get_inferred_urls() as you requested to test cocoapods PURL inputs with no name.

  • Also made a few changes to your suggested try/except approach -- I added try/except blocks to both get_download_url() and build_cocoapods_repo_url(), since get_inferred_urls() calls both get_repo_url and get_download_url and cocoapods only has repo_url support in purl2url.py (download_url support is now in fetchcode/package.py).

Note: these changes do not handle exceptions raised by other supported PURLs with no name, e.g., "pkg:cargo/abc": "https://crates.io/crates/abc", in test_purl2url_get_repo_url() raises an exception ValueError: purl is missing the required name component: 'pkg:cargo/'.

Question: since this output is consumed by the urls command in purldb-toolkit/purlcli.py, I don't want to raise an exception that will halt the underlying process of the urls command, e.g., a user might have submitted a list of 100 PURLs to query and does not want that to be interrupted. In purlcli.py and my fetchcode/package.py work I've used a "soft" fail approach that records an exception message in a shared .log file. I assume that's not an option here. As a placeholder I am printing the exception message to the console, but we do not want that in the PURLCLI tools. Do you have any suggestions?

TG1999
TG1999 previously approved these changes May 30, 2024
Copy link
Collaborator

@TG1999 TG1999 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@johnmhoran
Copy link
Member Author

@TG1999 You asked me to remove the two print statements I added to purl2url.py, one in get_download_url and the other in build_cocoapods_repo_url. Is this what we want for the cocoapods tests you asked me to add? E.g.,

def get_download_url(purl):
    """
    Return a download URL inferred from the `purl` string.
    """
    download_url = _get_url_from_router(download_router, purl)
    if download_url:
        return download_url

    # Fallback on the `download_url` qualifier when available.
    purl_data = None
    try:
        purl_data = PackageURL.from_string(purl)
    except Exception as e:
        # print(f"An error occurred in get_download_url(): {e}")
        return
    return purl_data.qualifiers.get("download_url", None)

Or are we perhaps contemplating something like this:

def get_download_url(purl):
    """
    Return a download URL inferred from the `purl` string.
    """
    download_url = _get_url_from_router(download_router, purl)
    if download_url:
        return download_url

    # Fallback on the `download_url` qualifier when available.
    purl_data = None
    try:
        purl_data = PackageURL.from_string(purl)
    except ValueError:
        return
    return purl_data.qualifiers.get("download_url", None)

Copy link
Member

@pombredanne pombredanne left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks. Do not swallow exception silently. Best is to let them propagate as-is when the purl is not valid.

@@ -75,7 +76,12 @@ def get_download_url(purl):
return download_url

# Fallback on the `download_url` qualifier when available.
purl_data = PackageURL.from_string(purl)
purl_data = None
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure we want to swallow an exception here. If the purl is invalid, let the error bubble up and do not catch the exception.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @pombredanne -- I have updated get_download_url() to the following (actually, this reverts the code to its state before I added a try/except):

def get_download_url(purl):
    """
    Return a download URL inferred from the `purl` string.
    """
    download_url = _get_url_from_router(download_router, purl)
    if download_url:
        return download_url

    # Fallback on the `download_url` qualifier when available.
    purl_data = PackageURL.from_string(purl)
    return purl_data.qualifiers.get("download_url", None)

I also removed "pkg:cocoapods/": [], from test_purl2url_get_inferred_urls() because otherwise the test fails w/o the try/except I've removed.

"""
purl_data = None
name = None
try:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as above: if the purl is invalid, let the error bubble up and do not catch the exception.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@pombredanne I'm not sure I understand your comments but hoping this is what you suggested. I've removed the try/except as well as this if condition:

    if not name:
        return

and the updated build_cocoapods_repo_url() looks like this:

@repo_router.route("pkg:cocoapods/.*")
def build_cocoapods_repo_url(purl):
    """
    Return a CocoaPods repo URL from the `purl` string.
    """
    purl_data = PackageURL.from_string(purl)
    name = purl_data.name
    return name and f"https://cocoapods.org/pods/{name}"

I also removed "pkg:cocoapods/": None, from test_purl2url_get_repo_url() because, having removed the try/except, that test fails on "pkg:cocoapods/": None, (which I'd added only to test the try/except).

@@ -24,6 +24,7 @@
# Visit https://github.com/package-url/packageurl-python for support and
# download.


Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove this

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks. I've deleted this:

# Visit https://github.com/package-url/packageurl-python for support and
# download.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That comment also appears at the top of /packageurl-python/tests/contrib/test_purl2url.py -- I've deleted it there as well.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I meant to remove the empty line.


if not name:
return
return f"https://cocoapods.org/pods/{name}"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use the simpler shortcut:

Suggested change
return f"https://cocoapods.org/pods/{name}"
return name and f"https://cocoapods.org/pods/{name}"

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated as shown in the comment just above ^.

…url#143

Reference: package-url#143

Signed-off-by: John M. Horan <johnmhoran@gmail.com>
@johnmhoran
Copy link
Member Author

@pombredanne @TG1999 I've just committed and pushed my changes in response to @pombredanne's recent comments. Please take a look when you can.

Reference: package-url#143

Signed-off-by: John M. Horan <johnmhoran@gmail.com>
@johnmhoran
Copy link
Member Author

@pombredanne I've restored the support/download URL comments we discussed.

pombredanne
pombredanne previously approved these changes Jun 7, 2024
Copy link
Member

@pombredanne pombredanne left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks. Looking good. Merging.

@@ -134,7 +136,10 @@ def test_purl2url_get_inferred_urls():
"https://gitlab.com/tg1999/firebase",
"https://gitlab.com/tg1999/firebase/-/archive/1a122122/firebase-1a122122.tar.gz",
],
"pkg:pypi/sortedcontainers@2.4.0": ["https://pypi.org/project/sortedcontainers/2.4.0/"],
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you restore the formatting as it was? The black tests are failing

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@pombredanne I've turned off Black (it had been set in 2 VSCode locations and turned off in just one) and restored the original format for that test entry you identified. Committed and pushed just now.

Reference: package-url#143

Signed-off-by: John M. Horan <johnmhoran@gmail.com>
@johnmhoran
Copy link
Member Author

@pombredanne Just confirming that this PR is ready for your re-review when you have the time.

@JonoYang
Copy link
Collaborator

@johnmhoran This looks good, merging.

@JonoYang JonoYang merged commit 04a706a into package-url:main Aug 12, 2024
5 of 19 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants